Skip to content

Fix global PM probes spawning bare names from the project (#421, #434, #438, #440) - #442

Merged
Mikola Lysenko (mikolalysenko) merged 12 commits into
mainfrom
agent/fix-global-probe-tool-spawn
Oct 2, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 12 commits into
mainfrom
agent/fix-global-probe-tool-spawn

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Fixes #421
Fixes #434
Fixes #438
Fixes #440

Summary

On Windows, scan -g, get -g and vex -g now find globally installed npm, yarn, pnpm, bun, RubyGems and Composer packages. Before this change they reported an empty, successful scan. The npm-family global lookups also no longer run inside the scanned project, so a Yarn Berry project's "global" script can't run or choose the directory that gets scanned (and patched) as the global install. Composer's global home also falls back to Composer's own platform defaults.

Priority note: the cluster is p1 (npm/yarn/RubyGems) and includes one p2 member (#438, Composer), because they share the same boundary.

Root cause

Global discovery asks each package manager where its global tree lives: npm root -g, yarn global dir, pnpm root -g, bun pm bin -g, gem env gemdir|gempath and composer global config home. Every one of these probes went through SystemCommandRunner::run (crates/socket-patch-core/src/utils/process.rs), which called Command::new(bin) on the bare tool name. That caused two problems:

Fix

  • SystemCommandRunner resolves the program with the existing resolve_tool (PATHEXT on Windows, absolute PATH entries only, so a tool planted in the project via . on PATH is never run) and spawns the resolved path through command_for. It never spawns the bare name.
  • On Windows, when resolve_tool finds nothing, resolve_app_alias_with looks for <name>.exe on absolute PATH entries only. It uses symlink_metadata, because an App Execution Alias such as the Store python3.exe is a reparse point that is_file can't follow. An earlier revision fell back to std's own search here, which walks relative PATH entries such as . against the parent's cwd, so it could run an executable planted in the project. That fallback is gone.
  • New GlobalProbeRunner runs the probe from a neutral directory: the user's home (absolute only), otherwise the drive root, and never the project. The npm, yarn, pnpm and bun global probes and composer global config home use it. gem env keeps the project cwd on purpose, because rbenv and chruby choose the Ruby from the project's .ruby-version, and local mode uses the same lookup.
  • get_composer_home now probes Composer's own defaults: %APPDATA%\Composer first on Windows, and ~/.composer, then $XDG_CONFIG_HOME/composer, then ~/.config/composer elsewhere. Relative or empty variables are ignored.
  • The existing Composer global tests now also unset APPDATA and XDG_CONFIG_HOME, as they already do for HOME and PATH, so a real Composer home on the test machine can't change their result.

Tests

The new suite crates/socket-patch-core/tests/global_probe_spawn_e2e.rs puts fake tools on PATH the way they install on each OS: an executable sh script on Unix, and a <name>.cmd shim with no .exe on Windows.

Issue Test Red without the fix
#434 npm_family_global_probes_find_the_installed_shims (npm/pnpm/bun) Windows only (shim not found). Passes on test (windows-latest)
#434 (yarn), #440 yarn_global_probe_runs_outside_the_scanned_project Linux, verified locally: left: "/tmp/…/proj", right: "/tmp/…/home". On Windows it also fails, because the shim isn't found. Passes on windows-latest
#440 (hardening) global_probe_ignores_a_tool_planted_on_a_relative_path_entry (Unix) Linux, verified locally: left: Ok("/planted/node_modules")
#440 (Windows fallback) global_probe_never_runs_an_executable_planted_in_the_project (Windows) A copy of cmd.exe planted as npm.exe in the project with only . on PATH. The old fallback would run it and return its banner as the prefix. Not executed locally (no Windows host); first run is on windows-latest
#421 global_gem_paths_come_from_the_installed_gem_shim Windows only (gem.cmd). Passes on windows-latest
#438 (1) composer_home_comes_from_the_installed_composer_shim Windows only (composer.cmd). Passes on windows-latest
#438 (2) composer_home_falls_back_to_xdg_config_home (Unix), composer_home_falls_back_to_appdata_on_windows (Windows) Linux, verified locally: left: []. The APPDATA test passes on windows-latest

There are also unit tests: neutral_probe_dir_prefers_an_absolute_home_and_never_the_cwd, and resolve_app_alias_skips_relative_entries (a yarn.exe reachable only through ., an empty entry or a bare dir name is never chosen; one on an absolute entry is).

Commands run locally (Linux) on dce7d1a:

  • cargo clippy --workspace --all-features -- -D warnings: clean
  • cargo test -p socket-patch-core --lib utils::process: 15 pass
  • cargo test -p socket-patch-core --test global_probe_spawn_e2e --test crawler_composer_e2e --test crawler_npm_e2e: 6, 33 and 82 pass
  • cargo test --workspace --all-features --no-fail-fast (earlier revision): everything passes except 12 permission-injection tests that fail only because this sandbox runs as uid 0. None touch the probe code; CI runs as non-root.
  • I couldn't type-check the Windows-only test locally: cross-compiling ring needs a MinGW toolchain this sandbox lacks. test (windows-latest) is its first build.
  • Formatting: main isn't rustfmt-clean under the pinned 1.93.1 toolchain, and CI doesn't gate on it, so I didn't apply a workspace-wide cargo fmt.
  • No wrapper changes are needed. npm/, pypi/ and gem/ only dispatch the binary.

Follow-ups (not in this PR)

🤖 Generated with Claude Code

https://claude.ai/code/session_01KKwBoKTsqpGR3ZcCAcEyCA


Note

Medium Risk
Changes how external package-manager CLIs are resolved and spawned for global discovery; mistakes could mis-target global install trees or skip valid tools, but scope is limited to probe/discovery paths with extensive regression tests.

Overview
Global mode (-g) now discovers machine-wide npm, yarn, pnpm, bun, RubyGems, and Composer installs reliably instead of often returning an empty scan.

Process spawning resolves package-manager binaries via resolve_tool (PATHEXT on Windows for .cmd/.bat shims) and spawns the resolved path; on Windows, resolve_app_alias_with can pick up App Execution Aliases on absolute PATH entries only, without falling back to bare names that could run project-local executables.

Global prefix probes for npm, yarn, pnpm, bun, and composer global config home use a new GlobalProbeRunner that runs from a neutral directory (absolute home, else drive root), so Yarn Berry cannot answer yarn global dir via a project "global" script and relative PATH entries cannot hijack the probe.

Composer global home fallbacks now follow Composer’s own defaults: %APPDATA%\Composer on Windows and XDG/~/.config/composer elsewhere, via extracted composer_home_candidates.

Changelog and e2e/unit tests cover shims, neutral cwd, PATH hardening, and Composer fallback ordering.

Reviewed by Cursor Bugbot for commit dce7d1a. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Global mode asked npm, yarn, pnpm, bun, gem and composer where their
global installs live by spawning the bare tool name. On Windows those
tools are .cmd/.bat shims that a bare spawn never finds, so scan -g
silently reported nothing to patch. The yarn probe also ran inside the
scanned project, where Yarn Berry runs the project's own "global"
script and its output picked the directory scanned as global.

Probes now resolve the tool through PATHEXT (and never from a relative
PATH entry), npm-family global probes run from the home directory, and
Composer's home falls back to %APPDATA%\Composer and
$XDG_CONFIG_HOME/composer like Composer does.

Fixes #421, #434, #438, #440.

Assisted-by: Claude Code:claude-opus-5-5
A Windows App Execution Alias (the Store python3.exe) is a reparse
point the PATH lookup can't stat, though a bare spawn launches it. The
Python probe shares this runner, so fall back to the bare name on
Windows when the lookup finds nothing, rather than lose an interpreter
that used to be found. Also satisfies clippy's redundant closure lint.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Two red checks on 3033eee, neither in code this PR touches:

  • e2e (ubuntu-latest, e2e_redirect_maven_build, 3.8.9) died in the fixture warm-up against Maven Central, before socket-patch ran: Could not find artifact org.apache.maven.plugins:maven-dependency-plugin:jar:3.6.1 in central. The diff doesn't touch Maven, and Maven discovery doesn't use the probe runner. I'll re-run it once when the CI run completes.
  • Pipenv matrix (macos-latest, …): one leg (2018.11.26 direct hosted, rollbackRestoresLockBytes) failed after PyPI request transport failed retries. The same leg passed on b29d415, and the only change since is behind cfg!(windows) plus a clippy lint, so macOS runs identical code. That's the transport-blip class Fix Pipenv matrix flake on 503/DNS transport blips #419 targets. I've re-run the failed job once.

If either fails again on re-run, I'll treat it as real and root-cause it.


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Global Composer discovery now also tries Composer's own defaults,
%APPDATA%\Composer and $XDG_CONFIG_HOME/composer. On a Windows runner
APPDATA points at a real Composer home, which outranks the ~/.composer
and ~/.config/composer candidates these tests stage. Unset both
variables there, as the tests already do for HOME and PATH, so they
keep exercising the HOME candidates.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

An earlier cargo fmt run over the whole workspace reformatted 125
files this change doesn't touch. Restore them to main so the diff
holds only the global-probe fix.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Ready for review on 059b07b: mergeable, 0 commits behind main.

  • CI: 405/405 check runs green on 059b07b (6 skipped by workflow conditions, none failing).
  • Bugbot: reviewed 059b07b, no new issues (earlier runs on 3033eee/a5ef5a0 are stale); 0 unresolved review threads.
  • Reviewer focus: the PATHEXT-aware tool resolution in crates/socket-patch-core/src/utils/process.rs and how the Composer/npm global probes now spawn resolved tools instead of bare names from the project cwd; new e2e coverage in tests/global_probe_spawn_e2e.rs.

Note: Slack announcement could not be sent this run (no Slack send tool available in the agent session); next run will retry.


Generated by Claude Code

Union the CHANGELOG Fixed entries from both sides.

Co-Authored-By: Claude <noreply@anthropic.com>
@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-core/tests/crawler_composer_e2e.rs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KKwBoKTsqpGR3ZcCAcEyCA
The no-composer and empty-HOME tests still inherited APPDATA and
XDG_CONFIG_HOME. Global discovery now probes %APPDATA%\Composer and
$XDG_CONFIG_HOME/composer, so a machine with a real Composer home
there made both tests see a vendor dir and fail. Unset both variables
in these tests too, as the sibling HOME-fallback tests already do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KKwBoKTsqpGR3ZcCAcEyCA
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

e2e (ubuntu-latest, mode_migration_vlt, 1.0.0-rc.14, …) failed on 465db05 before any test ran, and the cause isn't in this PR. Downloading the e2e-bin-ubuntu-latest artifact (146 MB) took about 1.5 minutes and left target/e2e-bin/ empty, so the next step failed with cp: cannot stat 'target/e2e-bin/socket-patch'. This PR doesn't touch the workflow or the artifact upload. I'll re-run the job once when the rest of CI run 36870777682 has finished; GitHub refuses a re-run while the run is still going.


Generated by Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KKwBoKTsqpGR3ZcCAcEyCA
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent check: ready for review at d60a204e.

  • CI: 97/97 non-skipped checks green on d60a204 (3 skipped)
  • Bugbot reviewed d60a204 and found no new issues. No review threads are open.
  • Reviewers: the last push was a merge of main to resolve a conflict; the fix itself didn't change since it was last reviewed.

Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: ready for review again at 0958e11 (0958e11aa1be45852ede833dae021232dbfdf521).

  • Merged main to clear a conflict. Only CHANGELOG.md conflicted, and both entries are kept. No code changes.
  • CI: all non-skipped checks green on 0958e11.
  • Bugbot: reviewed 0958e11, no new findings; all review threads resolved.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Reviewed 0958e11aa1be. Changes needed before merge.

[P1] Preserve absolute-PATH filtering in the Windows fallback — crates/socket-patch-core/src/utils/process.rs:207.

When resolve_tool finds no safe executable, None if cfg!(windows) => PathBuf::from(bin) hands the bare name back to Command. Rust 1.93.1's Windows search implementation skips empty PATH entries, but still searches relative entries such as .. It resolves the executable in the parent process before applying the child's current_dir.

Consequently, on Windows with . on PATH and no installed Yarn on an absolute PATH entry, a yarn.exe planted in the scanned project is still executed by the global probe. The neutral child cwd does not prevent it. Please preserve the filtered lookup when supporting App Execution Aliases—for example, probe only absolute candidates—and add a Windows case with a planted executable and no safe matching tool.

Validation: cargo test --locked -p socket-patch-core --test global_probe_spawn_e2e: 6 passed on macOS. The Windows finding is source-traced against the pinned Rust implementation and independently cross-checked; I did not execute the Windows reproduction locally.

When the safe lookup found no tool, the Windows fallback for App
Execution Aliases handed the bare name back to std. Its Windows search
also walks relative PATH entries such as '.', resolved against the
parent's cwd before the child's current_dir applies, so with '.' on
PATH an npm.exe or yarn.exe planted in the scanned project ran during
the global probe despite the neutral child cwd.

Look for the alias as <name>.exe on absolute PATH entries only, using
symlink_metadata since the reparse point can't be followed, and never
spawn the bare name. A Windows e2e test plants a copy of cmd.exe as
npm.exe in the project with only '.' on PATH and checks the global
npm probe finds nothing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KKwBoKTsqpGR3ZcCAcEyCA
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KKwBoKTsqpGR3ZcCAcEyCA
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Thanks, the [P1] is right. The old comment saying std never searches the cwd on Windows was wrong, because std still walks relative PATH entries such as .. Fixed in 96097f3, and main is merged in at dce7d1a:

  • The Windows fallback no longer hands the bare name to std. When resolve_tool finds nothing, the new resolve_app_alias_with looks for <name>.exe on absolute PATH entries only. It uses symlink_metadata, so App Execution Alias reparse points are still found, but nothing reachable only through ., an empty entry or a relative directory can be chosen. If there's no match, the probe returns None.
  • New Windows e2e test, global_probe_never_runs_an_executable_planted_in_the_project. It copies cmd.exe into the project as npm.exe, sets PATH to . only, chdirs into the project, and asserts get_npm_global_prefix() is an error. With the old fallback, std would find .\npm.exe, run it, and return the cmd banner as the "prefix".
  • New unit test, resolve_app_alias_skips_relative_entries, which runs on every OS.

I couldn't run or type-check the Windows test here: there's no Windows host, and cross-compiling ring needs MinGW. So test (windows-latest) on this push is its first real run, and I'll watch it. Clippy and the Linux suites pass. I've also corrected the PR description.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

test (ubuntu-latest) failed on dce7d1a in one library test: crawlers::npm_crawler::oracle::tests::walk_without_a_pool_matches_the_sequential_oracle (no pool, seed 12: find_by_purls differs under …/proj/node_modules). The two sides disagree on how far a .pnpm symlink cycle is followed (.pnpm/@s+b@github.com+u+r@abc1/node_modules/.pnpm/…).

I don't think this PR caused it:

  • This branch only changes npm_crawler.rs to swap SystemCommandRunner for GlobalProbeRunner in the four get_*_global_prefix helpers. The crawl, find_by_purls and the store walk the oracle compares are untouched.
  • dce7d1a is the first time this branch includes the npm crawler changes from main (Fix npm crawler missing relocated dependency stores (#359, #362) #365 and later).
  • The test passed 6 times out of 6 locally on this exact commit (cargo test -p socket-patch-core --lib crawlers::npm_crawler::oracle).
  • Every recent main CI run was cancelled by the next push, so main hasn't shown whether it passes there.

No fix exists for it yet. I'll re-run the failed job once this CI run finishes (GitHub refuses a re-run while the run is still going). If it fails again, I'll treat it as a real nondeterminism in the crawler-vs-oracle walk and look into it.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Follow-up reviewed dce7d1af663ab9e89f2c1b35fffc7916fe81edc0. The earlier P1 is fixed; code review is clear, pending CI clearance.

The Windows alias fallback now considers only absolute PATH entries and passes the selected absolute .exe path to Command. It no longer restores bare-name lookup, so Rust cannot select a project-planted executable through . or another relative PATH entry. The new Windows regression covers the original trigger. No new actionable issues found in this delta.

Validation: cargo test --locked -p socket-patch-core --lib utils::process 15 passed; cargo test --locked -p socket-patch-core --test global_probe_spawn_e2e 6 passed on macOS. I also checked the absolute-path branch of the pinned Rust Windows implementation. The Windows-only executable regression and actual Store alias launch were not run locally.

The final main-merge commit leaves the process implementation and probe tests identical to tested 96097f38; no additional PR-specific change was introduced.
The final head currently has a failing test (ubuntu-latest) check: walk_without_a_pool_matches_the_sequential_oracle differs on encoded scoped PURLs for seed 12. This is outside the process-execution change reviewed here, but the check still needs to pass before merging.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit dce7d1a. Configure here.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Re-verified Ready for review at dce7d1af663ab9e89f2c1b35fffc7916fe81edc0.

  • CI: 400/400 non-skipped checks green on the head (6 skipped). The earlier test (ubuntu-latest) npm-crawler oracle failure passed on re-run. That test is outside this PR's diff.
  • Bugbot: reviewed dce7d1a, no new issues. The earlier composer APPDATA isolation finding was fixed in 465db05 and resolved.
  • Mergeable: yes, no conflicts with current main. It is only waiting on approval.
  • For reviewers: the Windows alias fallback now resolves only absolute PATH entries to an absolute .exe. The Windows-only regression test runs in CI only.

Slack announcement not sent: the Slack connector in this session has no send-message tool.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Re-reviewed dce7d1af663ab9e89f2c1b35fffc7916fe81edc0. The earlier Ubuntu CI blocker has cleared; no additional code change is needed.

The Ubuntu test job now passes on this same head, as do the Windows and macOS test jobs. The Windows absolute-PATH security fix remains unchanged, and the branch merges cleanly with current main (73b17db5). No new actionable defect found.

Fresh local validation: 4 oracle tests, 15 process tests, and 6 global-probe integration tests pass on macOS, including the oracle test that previously failed in Ubuntu CI. The old failure cleared on a same-SHA CI rerun; the local replay also passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment